-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add the merge logic to BuildResult for handling RequestedProjectState, BuildFlags and ProjectInstance reliably #11765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add the merge logic to BuildResult for handling RequestedProjectState, BuildFlags and ProjectInstance reliably #11765
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a robust merge logic for BuildResult by improving project state merging and build flag handling to resolve issues with inconsistent project state after build operations. Key changes include:
- Enhancements in ProjectInstance to safely clone collections and properties, utilizing modern C# syntax.
- New merge methods in BuildResult to combine build flags and project state, including properties, items, and filter states.
- Expanded functionality and unit tests for RequestedProjectState to accurately merge property and item filters.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/Build/Instance/ProjectInstance.cs | Uses new C# constructs and null-safe operations when cloning collections and copying arrays. |
src/Build/BackEnd/Shared/BuildResult.cs | Adds merge methods for build flags and project state with local functions to merge properties and items. |
src/Build/BackEnd/Components/Caching/ResultsCache.cs | Refactors constructors and flag comparisons using concise lambda expressions. |
src/Build/BackEnd/BuildManager/RequestedProjectState.cs | Introduces a Merge() method to combine property and item filters using modern collection syntax. |
src/Build.UnitTests/BackEnd/BuildResult_Tests.cs | Provides thorough new tests validating the merged state behavior for project items, properties, and filters. |
Comments suppressed due to low confidence (1)
src/Build/BackEnd/Shared/BuildResult.cs:844
- [nitpick] Consider extracting the local merge functions (MergeProperties and MergeItems) into separate private methods to improve readability and maintainability.
void MergeProperties(ICollection<ProjectPropertyInstance> newProperties) { ... }
Co-authored-by: Copilot <[email protected]>
/// <returns>A merged ProjectInstance containing properties and items from both instances, or null if both instances are null.</returns> | ||
private ProjectInstance? MergeProjectStateAfterBuildInstances(BuildRequestDataFlags? newFlags, ProjectInstance? newProjectStateAfterBuild) | ||
{ | ||
if ((_buildRequestDataFlags & Execution.BuildRequestDataFlags.ProvideProjectStateAfterBuild) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the same sort of reasoning as above?
// If we have ProvideProjectStateAfterBuild set, it means we have a full project instance, so we don't need to merge anything.
{ | ||
foreach (var property in newProperties) | ||
{ | ||
if (mergedInstanceCandidate.GetProperty(property.Name) == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things here:
- I'm not sure about the way we capture the mergedInstanceCandidate. Can we do it in a more "visible" way please? as the function call currently stands, it doesn't look connected to the mergedInstanceCandidate.
- there is a chance that .GetProperty(property.Name) and .SetProperty(property.Name, property.EvaluatedValue); will lock the underlying collection. I'm not 100% sure since the code is structured in a weird way, however the
internal T this[string name]
in PropertyDictionary.cs locks.- is this a code path that will get locked? if no, good, if yes, do we care?
// Merge metadata from the new item into the existing one | ||
foreach (var metadata in item.EnumerateMetadata()) | ||
{ | ||
existingItem.SetMetadata(metadata.Key, metadata.Value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be doing multiple adds to an CopyOnWrite dictionary. Can we double check to make sure this is safe to do please? (This could be creating a bunch of expensive copies in the worst case)
ImportMetadata is unfortunately not exposed to the existingItem, so the fix (if needed) might not be straightforward)
d545849
to
b3931c5
Compare
Fixes #11701
Context
The PR addresses an issue where Visual Studio hangs during build operations, due to inconsistent project state merging after build operations. The root cause appears to be how
BuildResult
instances handle merging of project state when multiple build operations are combined (per the sameConfigurationId
).Why These Changes Were Made
The core issue appears to be inconsistent handling of project state during build operations, particularly when:
Multiple build operations need to be merged
Different build operations request different subsets of project state
The merge operation doesn't preserve all necessary metadata, properties, or flags
Prior to this change, when build results were merged using the MergeResults method, the logic didn't properly handle the merging of project state after build.
The new implementation explicitly merges:
Changes Made
MergeProjectStateAfterBuildInstances
: This method properly merges project instances, including their properties, items, and requested state filters. It handles cases where instances might be null, or when one instance has a full project state versus a partial state.MergeBuildFlags
: This method provides explicit control over how build flags are merged, with special handling for theProvideProjectStateAfterBuild
andProvideSubsetOfStateAfterBuild
flags.RequestedProjectState
with Merge CapabilityAdded a new
Merge()
method toRequestedProjectState
that properly combines property filters and item filters from two different instances.Implemented sophisticated merging logic for item filters that preserves metadata from both sources.
ProjectInstance
Enhanced the FilteredCopy constructor to handle null collections gracefully.
Updated code to use modern C# collection initializers and null-safe operations.
Made the handling of various dictionaries (properties, items, etc.) more robust against null values.
ResultsCache.AreBuildResultFlagsCompatible
If a subset of data is requested, we don't need to rely on the cache flag, but check if the available subset of the data can satisfy the request.
Testing
Added UTs to cover new Merge Logic
Notes